feat(node): Add maxCacheKeyLength to Redis integration (remove truncation)
#18045
Conversation
s1gr1d
added
the
Integration: redis
| describe('early returns', () => { | ||
| it.each([ | ||
| { desc: 'no args', cmd: 'get', args: [], response: 'test' }, | ||
| { desc: 'unsupported command', cmd: 'exists', args: ['key'], response: 'test' }, | ||
| { desc: 'no cache prefixes', cmd: 'get', args: ['key'], response: 'test', options: {} }, | ||
| { desc: 'non-matching prefix', cmd: 'get', args: ['key'], response: 'test', options: { cachePrefixes: ['c'] } }, | ||
| ])('should always set sentry.origin but return early when $desc', ({ cmd, args, response, options = {} }) => { | ||
| vi.clearAllMocks(); | ||
| Object.assign(_redisOptions, options); | ||
|
|
||
| cacheResponseHook(mockSpan, cmd, args, response); | ||
|
|
||
| expect(mockSpan.setAttribute).toHaveBeenCalledWith('sentry.origin', 'auto.db.otel.redis'); | ||
| expect(mockSpan.setAttributes).not.toHaveBeenCalled(); | ||
| expect(mockSpan.updateName).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I added some general tests for the handler, as there were none for this behavior so far.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| { desc: 'no cache prefixes', cmd: 'get', args: ['key'], response: 'test', options: {} }, | ||
| { desc: 'non-matching prefix', cmd: 'get', args: ['key'], response: 'test', options: { cachePrefixes: ['c'] } }, | ||
| ])('should always set sentry.origin but return early when $desc', ({ cmd, args, response, options = {} }) => { | ||
| vi.clearAllMocks(); |
There was a problem hiding this comment.
super-l: Maybe it is more future proof to move this into the afterEach
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Left a comment, other than that looks good!
| span.updateName(truncate(spanDescription, 1024)); | ||
| span.updateName( | ||
| _redisOptions.maxCacheKeyLength ? truncate(spanDescription, _redisOptions.maxCacheKeyLength) : spanDescription, | ||
| ); |
There was a problem hiding this comment.
Adding a new comment here since the previous one was marked as outdated. Setting maxCacheKeyLength to 0 will result in not truncating at all, which would be unexpected IMO.
There was a problem hiding this comment.
It's the default behavior of our truncate function and I didn't want to do it different here. I will add a note in the JS doc of the Redis instrumentation to make it clear what passing 0 does here.
This removes the automatic truncation of the span description. Now, all cache keys are set in the description.
part of #17389